PYTHON-3606 - Document best practice for closing MongoClients and cursors #2465
Conversation
pymongo/asynchronous/cursor.py
Outdated
| raise StopAsyncIteration | ||
| if len(self._data) or await self._refresh(): | ||
| return self._data.popleft() | ||
| else: | ||
| if self._cursor_type == CursorType.NON_TAILABLE: | ||
| await self.close() |
There was a problem hiding this comment.
I believe this close is redundant since _send_message() already calls close after receiving the final batch. Or is there another case where the cursor would be left open?
There was a problem hiding this comment.
So the use case when a cursor is not automatically closed (outside of a context manager) is when a user doesn't fully iterate through it?
There was a problem hiding this comment.
Yeah only an non-exhausted cursor is left open. Even then the cursor is closed at a later point by our GC hooks (via the kill cursor thread) but closing explicitly has stronger guarantees.
There was a problem hiding this comment.
In that case, I think a docstring update alone is sufficient. Explicitly calling out that if you don't fully iterate a cursor, you should close it explicitly feels much more actionable than "always use a context manager".
There was a problem hiding this comment.
Docs only SGTM. It could explain the close behavior and then say something like "The best practice to ensure the cursor is always closed promptly is to use a context manager..."
pymongo/asynchronous/cursor.py
Outdated
| @@ -1263,10 +1263,14 @@ async def next(self) -> _DocumentType: | |||
| self._exhaust_checked = True | |||
| await self._supports_exhaust() | |||
| if self._empty: | |||
| if self._cursor_type == CursorType.NON_TAILABLE: | |||
| await self.close() | |||
There was a problem hiding this comment.
Is this a bug fix? If so should we create a new bug ticket for it?
There was a problem hiding this comment.
I'll open a ticket for this. Empty cursors should always be immediately closed.
pymongo/asynchronous/mongo_client.py
Outdated
| Best practice is to call :meth:`AsyncMongoClient.close` when the client is no longer needed, | ||
| or use the client in a with statement:: | ||
|
|
||
| with AsyncMongoClient(url) as client: |
There was a problem hiding this comment.
Should this be "async with"?
| @@ -1776,6 +1776,15 @@ def find(self, *args: Any, **kwargs: Any) -> AsyncCursor[_DocumentType]: | |||
| improper type. Returns an instance of | |||
| :class:`~pymongo.asynchronous.cursor.AsyncCursor` corresponding to this query. | |||
| Cursors are closed automatically when they are exhausted (the last batch of data is retrieved from the database). | |||
There was a problem hiding this comment.
Should we include this note in any other places? Like Cursor or CommandCursor?
There was a problem hiding this comment.
We explicitly say users shouldn't create those classes directly. Is adding this still useful for our own internal documentation?
There was a problem hiding this comment.
I'm wondering because this is only in find() but there are other methods that create cursors, the most important being aggregate().
There was a problem hiding this comment.
Good catch. I'll add it to all public methods that return a Cursor or CommandCursor.
Note: this PR contains both documentation changes and behavioral changes. Only one of these approaches should be ultimately taken, but both are included here for easy comparison.